Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proxy Update #6

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Conversation

xn4224nx
Copy link

@xn4224nx xn4224nx commented Feb 5, 2023

Introduction

This update’s primary purpose is to allow the module to work on a system that requires a proxy to be supplied to any request to the Fingertips API.

This update modifies every function that makes a direct request to the Fingertips API adding a parameter called proxy that is given to the get request. This parameter is also added to every function that uses functions that make direct requests and so on and so forth.

In addition to the major purpose of the update it also encompasses a certain degree of maintenance and fixes to get the package up to standard. Nearly every files has been changed in someway.

Changes Made to api_calls.py

All functions have a new parameter called proxy that has a default value of None. This value is passed to all instances of the request.get() function as the proxies parameter of the get function. For each function the doc string was updated.

A new function was added get_csv(url, proxy=None) that returns a pandas dataframe from the url to a csv file and a corresponding test was created in the relevant place.

Changes Made to area_data.py

The function defined_qcut() has been removed as this is now obsolete as the pandas module contains an implementation.

The deprivation_decile function has also been heavily refactored to take account of the new deprivation indicator data as well as dynamically obtaining the deprivation indicators and giving more detailed errors about the viable combinations that the function and the API accept.

Changes Made to metadata.py

In addition to adding the proxy parameter being added to every function the get_metadata_for_all_indicators() function has been altered. In its previous incarnation it did not expand columns of JSON objects but this has changed so the output is a flat dataframe without complex columns. In addition the parameters of the functions have been changed so boolean variables determine the binary choices of the function rather than string types.

This is a major change as old usages of the function will fail as the attributes of the function have become incompatible and the output has fundamentally changed in form. But this has been done as the original implementation outputted a dataframe that would need further processing before being usable by most other functions in the pandas library. The function parameters can be changed back with minimal issue but the new version ensures that new users will not make syntax errors from misspelling or incorrect capitalisation.

Also a number of functions have been refactored to remove the depreciated pandas function append() and replace it with concat().

Changes Made to retrieve_data.py

Minimal changes comprising adding the proxy variable and refactoring code that used depreciated functions.

Changes Made to tests.py

In the current version of the code there are thirty six tests and nine of them currently fail. The changes made to this script are to correct this by altering the tests that fails so they better reflect the current state of the API. Some of the common changes are to remove length checks and rather confirm that some of the key columns that should be present are. Other modifications check that certain columns contain only particular values rather than checking the dimension of the dataframe returned by the function.

Tests that pass have not been modified but changes have been made to scripts to ensure warnings are not issued.

Changes to Documentation

Sphinx has been rerun to ensure it is up to date with the new functions and parameters. The readme.md file has also been altered to use python highlighting amongst other things.

Why Am I Doing This

I am a Public Health Infomation Analyst working at Norfolk County Council and a prolific Fingertips and python user. Unfortunately when using python we have to supply proxy information to get it to access the internet, the fingertips_py module doesn’t currently have this functionality so I am trying to add it.

I am relatively new to git and Github so please forgive me if I am going about this the wrong way or have committed some Github faux pas!

Feel free to pick and choose these changes and if something won’t fit or is not suitable please say and I will be more than happy to rework it.

benjamin-mouncer and others added 30 commits January 23, 2023 21:06
… expanded out and the function uses bool inputs rather than strings.
@sebastian-fox
Copy link
Contributor

Hi @xn4224nx - thank you for looking at this. We'll need a bit of time to review this, but the intention is helpful.

I think we need to provide some detail about "How to contribute" in the README, so what you've done is ok.
Generally, I know for a lot of open source packages, people will raise an issue. The package owner will then work with the person to determine what needs doing, or if it is even in scope for the package, and then you go from there. Like I say though, we would need to provide that guidance.

Thanks again, and we'll come back to you if we have any questions.

@xn4224nx
Copy link
Author

xn4224nx commented Feb 8, 2023

Hi @sebastian-fox you are very welcome.

No rush on any of this of course. I think your readme is fine personally, its just that I needed to read GitHub's own guidance on this matter. Which, as you say, advises contributors to open an issue first!

But thank you for being understanding 😊.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants